[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: elijah-rou
Once this PR has been reviewed and has the lgtm label, please assign dprotaso for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Hi @elijah-rou. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/ok-to-test

@elijah-rou: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Your PR dashboard.

Specifically, they have voiced need for a feature that is able to ensure that n instances are always available to serve requests, above what the autoscale suggests. This ensures that even at low volumes, there is enough capacity.

Any chance you can elaborate on the problem more?

Specifically, they have voiced need for a feature that is able to ensure that n instances are always available to serve requests, above what the autoscale suggests. This ensures that even at low volumes, there is enough capacity.

Any chance you can elaborate on the problem more?

There are many clients of ours that wish to maintain static capacity so that they can handle burst effectively. These are concurrency=1, GPU workload scenarios. At low traffic volumes, they would like n machines to be available to handle load, but cannot rely on purely proportional scaling through the scaling target at low enough volumes. They also do not want to overprovision capacity.

For eg: I know my traffic roughly varies in the 10 node range. ie at any point, 10 more requests may come in on the scaling evaluation interval. If I set my buffer to 10, that means that if I have 0 in-flight requests, I will have 0+10 machines total. If I have 60 in-flight requests, I then have 60+10 machines total. This buffer maintains even as requests grow. This works in tandem with the autoscaler, which is where this becomes useful. Say I have set target_utilization to 90 (as concurrency=1, this means effectively that I want to provision 10% more machines than requests I serve. So if I have 60 inflight here, I would have provisioned 66 total machines. At low volumes, this may not be enough buffer to handle usual traffic growth, so if I introduce a buffer of 10 here, what scale_buffer will do is simply add n statically to the suggested autoscaler amount (in our eg, 60+(6+10) = 76, machines). This ensures at low volumes that I have enough capacity, but at high-volumes I do not overscale by setting scaling_target to a lower number (80%, 70%, etc).

Unsure if you're aware we have an activation scale knob - so when scaling from zero it will hit a min number of replicas

See: https://knative.dev/docs/serving/autoscaling/scale-bounds/#scale-up-minimum

Though it will eventually scale down based on the request load - there was a discussion about that here: #14017

so that they can handle burst effectively.

We also have target burst capacity knob as well
https://knative.dev/docs/serving/load-balancing/target-burst-capacity/#setting-the-target-burst-capacity

I am aware of both of these, neither cater for the use case I am proposing. Scale up activation only works from zero, it does not help when we have already scaled. Target burst does not help either, we do not want requests to be queued onto targets. These requests can take anything from 5 minutes to 3 hours, and concurrency is 1 so it will block target burst queued requests. If a request prematurely queues, it can get stuck on an instance indefinitely. Target burst capacity in this instance must be -1 (there is no target burst capacity). This feature predominately addresses concurrency=1 scenarios for GPU workloads.

Hi @elijah-rou, If I understand correctly, the proposal is to keep a buffer of replicas around in order to make sure you have enough capacity to guarantee that latency is acceptable (no queues) and absorb spikes. One request per replica is because of model restriction or resources?

In any case, buffer based capacity management and autoscaling is already a method used elsewhere where capacity is always above demand at any given time. I think it would make more sense to have a buffer percentage and not a static value as it would not depend on traffic patterns. I understand though that replicas are attached to gpus and you want to control the number somehow. Also it would be preferable to be able to to adjust the buffer percentage based on the volatility (changes in variance) and cover changing spike patterns. Here it seems things are static aiming for the buffer that covers the specific workload demands. I guess the goal with the proposal here is to adjust the buffer externally via some tooling per service.

To summarize you want to roughly follow the demand curve and you use (KPA + a static buffer) to implement it.
I think that predictive autoscaling is a better fit (#15068) in this scenario, for certain workloads of course. We added in the past the scale_down_delay option and I think we need another for proactive scale_up_speed_up that would be dynamically adjusted (one idea) so when scaling up you can quickly adjust with the demand that is predicted and not wait for kpa. However, in order not to block this PR we could have it behind a flag and iterate. cc @dprotaso

PR needs rebase.

I am impartial to whether we merge this. Entirely depends on if you guys feel users can make use of it or if there is indeed something more suitable (I would be happy for some sort of dynamic percentage based on both volatility & volume as you mentioned instead for eg). I am happy to use this as a starting point to tackle #15068 instead, and I can aim for 1.19 release (since 1.18 is round the corner)

I am impartial to whether we merge this.

I think we shouldn't merge and discuss (in the issue) what would have more broader appeal. I think supporting just scale + N is a bit narrow. I don't know what the right answer is though - it might be dependent on domain similar to how there's LLM metric aware routing - https://github.com/kubernetes-sigs/gateway-api-inference-extension

I also don't want to make serving overly complex - the goal is to simplify things for app developers. So there's a balance to be had.